care/emr/api/viewsets/patient.py (2)

403-420: Authorization checks look appropriate, but there's a subtle edge case to consider.

The facility-context check on line 407 only triggers when both request_config.facility and facility are present. If the request omits a facility parameter, facility will be None, and this check silently passes.

Fortunately, the second check (lines 413-420) validates user permission against request_config.facility regardless, so the authorization is still enforced. Just worth noting that the first check is more of a context-consistency validation than a security gate.

One thought: should the endpoint require a facility context when the config has a facility? Currently, if request_config.facility is set but no facility context is provided, the request proceeds to the permission check (which will likely fail anyway). It might be cleaner to be explicit:

💡 Optional: Make facility context required when config has facility

         # Ensure identifier config belongs to the patient's facility context
-        if request_config.facility and facility and request_config.facility.id != facility.id:
+        if request_config.facility:
+            if not facility:
+                raise PermissionDenied(
+                    "Facility context is required for facility-scoped identifiers"
+                )
+            if request_config.facility.id != facility.id:
+                raise PermissionDenied(
+                    "Identifier configuration does not belong to the patient's facility"
+                )

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@care/emr/api/viewsets/patient.py` around lines 403 - 420, The current logic
allows requests where request_config.facility is set but the incoming facility
context (variable facility from get_serializer_list_context()) is missing; add
an explicit guard so when request_config.facility is present and facility is
None you raise a clear error (e.g., ValidationError or PermissionDenied) stating
that a facility context is required for facility-scoped identifier configs; keep
the existing
AuthorizationController.call("can_write_facility_patient_identifier_config",
self.request.user, request_config.facility) check intact so permission
enforcement still runs afterward.

---

407-420: Consider extracting exception messages to constants (per static analysis hint).

The Ruff TRY003 warnings suggest avoiding long inline messages. While not critical, extracting these to constants would be marginally more maintainable—if you ever felt so inclined.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@care/emr/api/viewsets/patient.py` around lines 407 - 420, Extract the inline
PermissionDenied messages into module-level constants and use those constants in
the raises: create descriptive constants like IDENTIFIER_CONFIGURATION_MISMATCH
= "Identifier configuration does not belong to the patient's facility" and
CANNOT_UPDATE_IDENTIFIER_FOR_FACILITY = "Cannot update identifier for this
facility" (or similar) at the top of the patient.py module, then replace the
string literals inside the PermissionDenied raises in the block around
request_config/facility checks and the AuthorizationController.call check to
reference those constants instead.

care/emr/tests/test_patient_security_api.py (2)

20-25: Permissions granted don't include the one being checked.

The test grants can_write_patient and can_list_patients, but the authorization in patient.py checks for can_write_patient_identifier_config (via can_write_facility_patient_identifier_config).

The test still validates the 403 behavior correctly (since the user lacks the required permission entirely), but a more precise test would grant the correct permission for Facility A and verify it's denied for Facility B. Just something to consider for test clarity.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@care/emr/tests/test_patient_security_api.py` around lines 20 - 25, The test
currently grants PatientPermissions.can_write_patient and can_list_patients but
the authorization in patient.py checks for the facility-scoped permission
(can_write_facility_patient_identifier_config); update the role creation in
create_role_with_permissions to grant
PatientPermissions.can_write_facility_patient_identifier_config.name scoped to
Facility A (so the user has the correct permission for Facility A) and then
explicitly call the API for Facility B to assert a 403, ensuring the test
verifies allowed-for-A but denied-for-B behavior; reference
create_role_with_permissions, PatientPermissions and the facility check in
patient.py to locate the code to change.

---

16-17: org_b is created but never used.

Perhaps it was intended for a future test case, or it slipped through during implementation. Either way, it's just sitting there, not doing much.

🧹 Proposed cleanup

         self.org_a = self.create_organization(org_type="govt")
-        self.org_b = self.create_organization(org_type="govt")

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@care/emr/tests/test_patient_security_api.py` around lines 16 - 17, The test
creates self.org_b via create_organization but never uses it; remove the unused
setup to clean the test by deleting the self.org_b =
self.create_organization(org_type="govt") line (and any references to self.org_b
in setUp/teardown) in test_patient_security_api.py, or if it was intended to be
used, replace its removal by actually using self.org_b in the relevant
assertions — locate the creation in the test setup where create_organization is
called to make the change.

care/emr/tests/test_patient_security_api.py (2)

19-25: Indentation of the permissions list is inconsistent.

The list items appear to have drifted leftward, which doesn't quite align with PEP 8 conventions. I'm sure it was just a minor slip.

🧹 Suggested formatting fix

         self.role = self.create_role_with_permissions(
             permissions=[
-    PatientPermissions.can_write_facility_patient_identifier_config.name,
-    PatientPermissions.can_write_patient.name,
-    PatientPermissions.can_list_patients.name,
-    ]
+                PatientPermissions.can_write_facility_patient_identifier_config.name,
+                PatientPermissions.can_write_patient.name,
+                PatientPermissions.can_list_patients.name,
+            ]
         )

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@care/emr/tests/test_patient_security_api.py` around lines 19 - 25, The
permissions list passed to create_role_with_permissions is mis-indented; adjust
the bracketed list so each item
(PatientPermissions.can_write_facility_patient_identifier_config.name,
PatientPermissions.can_write_patient.name,
PatientPermissions.can_list_patients.name) is indented consistently and aligned
under the opening bracket per PEP 8 (same indentation level for each element)
and ensure the closing bracket lines up with the start of the argument list in
the create_role_with_permissions call.

---

13-13: Remove the unused facility_a variable for clarity.

The variable is created but never referenced in the test. Only facility_b is actually used in test_update_identifier_bola_vulnerability. The setup comment "User has access to Facility A" appears to be vestigial—the user's access is controlled through self.org_a and the permission system, not facility_a. Removing it would eliminate the confusion.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@care/emr/tests/test_patient_security_api.py` at line 13, The test setup
creates an unused facility variable self.facility_a via create_facility which is
never referenced—remove the unused assignment to self.facility_a from the setup
so only the needed facility_b remains; update any setup comments (e.g., "User
has access to Facility A") to reflect that user access is controlled via
self.org_a and the permission system, and ensure
test_update_identifier_bola_vulnerability still uses self.facility_b and
self.org_a as before.